Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Blocks: Add Ribbon to indicate Paid blocks in block picker #13490

Closed
wants to merge 2 commits into from

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Sep 18, 2019

WIP, currently untested.

Changes proposed in this Pull Request:

Add a ribbon to visually indicate paid blocks in the block picker

Is this a new feature or does it add/remove features to an existing part of Jetpack?

It does add a feature to an existing part of Jetpack 😬

Testing instructions:

TBD

Proposed changelog entry for your changes:

Blocks: Add Ribbon to indicate Paid blocks in block picker

@ockham ockham added [Status] In Progress [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Sep 18, 2019
@ockham ockham requested a review from a team September 18, 2019 10:37
@ockham ockham self-assigned this Sep 18, 2019
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello ockham! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D32892-code before merging this PR. Thank you!

@jetpackbot
Copy link

jetpackbot commented Sep 18, 2019

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 73eccab

@matticbot
Copy link
Contributor

ockham, Your synced wpcom patch D32892-code has been updated.

@ockham
Copy link
Contributor Author

ockham commented Sep 19, 2019

I've bumped calypso-build to 4.0.1 and added calypso-ui. Upon yarn build, I'm now getting

Error: [BABEL] ~/src/jetpack/gulpfile.babel.js: Cannot find module '@babel/plugin-proposal-class-properties' from '~/src/jetpack'
    at Function.module.exports [as sync] (~/src/jetpack/node_modules/resolve/lib/sync.js:71:15)
    at resolveStandardizedName (~/src/jetpack/node_modules/@babel/core/lib/config/files/plugins.js:101:31)
    at resolvePlugin (~/src/jetpack/node_modules/@babel/core/lib/config/files/plugins.js:54:10)
    at loadPlugin (~/src/jetpack/node_modules/@babel/core/lib/config/files/plugins.js:62:20)
    at createDescriptor (~/src/jetpack/node_modules/@babel/core/lib/config/config-descriptors.js:154:9)
    at items.map (~/src/jetpack/node_modules/@babel/core/lib/config/config-descriptors.js:109:50)
    at Array.map (<anonymous>)
    at createDescriptors (~/src/jetpack/node_modules/@babel/core/lib/config/config-descriptors.js:109:29)
    at createPluginDescriptors (~/src/jetpack/node_modules/@babel/core/lib/config/config-descriptors.js:105:10)
    at plugins (~/src/jetpack/node_modules/@babel/core/lib/config/config-descriptors.js:40:19)

Even though that package is a dependency of calypso-build.

This seems somewhat similar to p1566905517230900-slack-luna, which we hoped to fix with #13343 🤔

Installing that package directly gives the same kind of error for @babel/plugin-syntax-dynamic-import, which is also in calypso-build's dependencies 🤦‍♂️

package.json Outdated Show resolved Hide resolved
@ockham ockham force-pushed the add/paid-blocks-ribbon branch 2 times, most recently from 614232e to fbd5fa2 Compare October 10, 2019 06:52
@ockham
Copy link
Contributor Author

ockham commented Oct 10, 2019

Rebased, and bumped calypso-ui to 2.0.0-alpha.0. Getting the following error during yarn build:

        ERROR in ./node_modules/@automattic/calypso-ui/dist/esm/card/style.scss (./node_modules/css-loader/dist/cjs.js??ref--5-1!./node_modules/postcss-loader/src??ref--5-2!./node_modules/@automattic/calypso-build/node_modules/sass-loader/lib/loader.js??ref--5-3!./node_modules/@automattic/calypso-ui/dist/esm/card/style.scss)
        Module build failed (from ./node_modules/@automattic/calypso-build/node_modules/sass-loader/lib/loader.js):
        
        
                 ^
              No mixin named clear-fix
              in ~/src/jetpack/node_modules/@automattic/calypso-ui/dist/esm/card/style.scss (line 11, column 11)

This is because of https://github.com/Automattic/wp-calypso/blob/9ebe145354a7e7ab5540ce2ecd861c61ff7f963b/packages/calypso-ui/src/card/style.scss#L10 which needs https://github.com/Automattic/wp-calypso/blob/9ebe145354a7e7ab5540ce2ecd861c61ff7f963b/client/assets/stylesheets/shared/mixins/_clear-fix.scss.

Ad-hoc solution idea: Bundle mixins in a new package (calypso-sass-mixins?) and make them a dep for calypso-ui? Thoughts @sirreal @Automattic/team-calypso ?

@simison
Copy link
Member

simison commented Oct 10, 2019

Why not include the mixins (and possible animations) file in calypso-ui package?

@sirreal
Copy link
Member

sirreal commented Oct 10, 2019

Some exploratory work was done here, you should be able to use this published package as the sas loader prelude:

Automattic/wp-calypso#36170

That may fix the issues but isn't really well thought out.

@ockham
Copy link
Contributor Author

ockham commented Oct 10, 2019

Some exploratory work was done here, you should be able to use this published package as the sas loader prelude:

Automattic/wp-calypso#36170

That may fix the issues but isn't really well thought out.

Ah right, had forgotten about that PR.

Right off the bat: We're injecting the prelude at Webpack config level (through the SASS "preset") there, which is something I'd rather avoid for projects using @automattic/calypso-ui. Can we 'simply' change @automattic/calypso-ui itself to use the prelude?

@simison
Copy link
Member

simison commented Oct 10, 2019

Similar issues highlighted by Automattic/wp-calypso#36471

@jsnajdr
Copy link
Member

jsnajdr commented Oct 10, 2019

Can we 'simply' change @automattic/calypso-ui itself to use the prelude?

The prelude is used only when compiling SASS down to CSS, and therefore needs to be included at the place that does that compilation.

If the published calypso-ui package include only compiled CSS files (and not the SCSS ones), then the prelude could be its dev dependency.

But if the compilation is happening outside calypso-ui, then the prelude needs to be used there.

Publishing our Calypso base styles, i.e.,

  • the CSS reset stylesheet
  • the CSS stylesheet for form elements (we style even plain input elements that don't have any CSS classes)
  • shared animations (keyframe definitions)
  • SCSS variable and mixin definitions of all kinds

as @automattic/calypso-base-styles would be a great idea! 🚀

One of incremental steps is cleanup of @automattic/calypso-color-schemes usage in calypso-build in Automattic/wp-calypso#36471. Please have a look at that PR!

@ockham ockham force-pushed the add/paid-blocks-ribbon branch 2 times, most recently from 574031d to 0dfdd67 Compare November 11, 2019 07:14
@matticbot
Copy link
Contributor

ockham, Your synced wpcom patch D32892-code has been updated.

@@ -49,6 +51,14 @@ export default function registerJetpackBlock( name, settings, childBlocks = [] )
title: betaExtensions.includes( name ) ? `${ settings.title } (beta)` : settings.title,
edit: requiredPlan ? wrapPaidBlock( { requiredPlan } )( settings.edit ) : settings.edit,
example: requiredPlan ? undefined : settings.example,
icon: requiredPlan ? (
<>
<Ribbon>{ __( 'Paid' ) }</Ribbon>
Copy link
Member

@simison simison Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably give something little more for screen reader users than "paid", like "block name (requires paid plan)"

@jeherve
Copy link
Member

jeherve commented Mar 6, 2020

closing in favor of #14739

@jeherve jeherve closed this Mar 6, 2020
@ockham ockham deleted the add/paid-blocks-ribbon branch July 9, 2020 13:44
@jeherve jeherve added the [Block] Pay With Paypal aka Simple Payments label Feb 12, 2021
@jeherve jeherve added the [Feature] Pay with PayPal aka Simple Payments label Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Pay With Paypal aka Simple Payments [Block] Payment Button aka Recurring Payments [Feature] Pay with PayPal aka Simple Payments [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants